Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: permission before tool call #1313

Merged
merged 19 commits into from
Feb 24, 2025
Merged

feat: permission before tool call #1313

merged 19 commits into from
Feb 24, 2025

Conversation

wendytang
Copy link
Collaborator

@wendytang wendytang commented Feb 20, 2025

Internal testing instructions:

pull the branch

cd ~/Development/goose
git fetch && git checkout wtang/pbtc_branch

use env var

export GOOSE_MODE=approve # auto, approve, chat
cargo build
./target/debug/goose session

use config

# add GOOSE_MODE=approve in ~/.config/goose/config.yaml # auto, approve,chat 
./target/debug/goose session

Copy link
Collaborator

@baxen baxen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Have a few tweaks below but excited to try this

}
}
},
"chat" => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think instead of skipping tool calls in the loop, we really want to not advertise the tools to the agent at all? e.g. remove them from the call to provider.complete - otherwise i could see this leading to confusing dialogue

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subtle detail, but the ToolConfirmationRequest is not added to the agent loop and is not visible to the agent. Added some context in the chat mode to allow the agent to frame the tool call requests as plans.

@wendytang wendytang marked this pull request as ready for review February 24, 2025 16:55

// Wait for confirmation response through the channel
let mut rx = self.confirmation_rx.lock().await;
if let Some((req_id, confirmed)) = rx.recv().await {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to add any waiting deadline?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for cli, we shouldn't add a timeout imo, but for GUI, it should be considered

// Wait for confirmation response through the channel
let mut rx = self.confirmation_rx.lock().await;
if let Some((req_id, confirmed)) = rx.recv().await {
if req_id == request.id {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious, will we have the case that req_id != request.id

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, the identical message id should be passed back.

@wendytang wendytang merged commit 3723c64 into main Feb 24, 2025
6 checks passed
@wendytang wendytang deleted the wtang/pbtc_branch branch February 24, 2025 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants